Replace the concept of statement fetch modes with the explicit API #4007

morozov · 2020-05-13T01:59:22Z

Q	A
Type	improvement
BC Break	hell yes

The approach of using FetchMode when fetching data from a statement inherited from PDO has certain downsides:

The type of the return value of a given method (e.g. fetch()) is determined at runtime (it can be a mixed, array<int,mixed>, array<string,mixed> and even array<int|string,mixed>) and is unclear for static analysis.
Besides the mode passed explicitly to a fetch() method, it can be set on the statement via setFetchMode(), so if a component gets passed an existing ResultStatement(), it cannot be certain of the fetched data type unless it specifies it explicitly.
Even if the component executes the statement itself, it cannot be certain about the fetched data type because it can be set globally at the connection level.
Having the fetch mode expressed via a set of constants requires their additional validation in the method and throwing an exception if the mode is invalid or unsupported. This puts us in an awkward position: a lot of methods can throw an invalid mode exception but it's not handled anywhere.
Unlike calling fetch methods where a method can be passed explicitly, it's impossible to pass the fetch mode to the statement when iterating over it. It's only possible to set it on the statement and therefore modify the shared state that others may rely on.

The proposed solution is to define an explicit fetch-oriented API that driver-level statements have to implement. This API will define a dedicated method for each combination of the following:

Row presentation mode (associative array, numeric array, the value of the first column).
Row traversal mode (one row, all rows, iterator).

Statement	Numeric	Associative	One/Column	All	Iterator
`mysqli`	✓
`pdo`	✓	✓	✓	✓	✓
`sqlsrv`	✓	✓
`oci`	✓	✓	✓	✓
`ibm_db2`	✓	✓
Cache		✓		✓	✓

The above matrix illustrates the driver capabilities that I want to be exposed via the new API and is meant to justify its verbosity:

Numeric. Support for fetching rows as numeric arrays.
Associative. Support for fetching rows as associative arrays.
One/Column. Support for fetching one value or a column.
All. Support for all rows in one call.
Iterator. Support for iteration over rows.

As one can see, the numeric and associative modes are supported by most of the drivers and they are really the essential primitives for fetching data. The “one”, “all” and “iterator” capabilities are supported only by a few but I believe it's important to be able to use their in-driver implementation instead of having to emulate them in DBAL even if the driver supports it (see a similar problem in #3744).

Breaking changes:

The FetchMode class and the setFetchMode() methods are removed as well as the entire concept of the fetch mode.
The fetch() method is replaced with fetchNumeric(), fetchAssociative() and fetchOne() (like fetchOne() in ZF1).
The fetchAll() method is replaced with fetchAllNumeric(), fetchAllAssociative() and… should it be fetchColumn() like fetchCol() in ZF1?
Connection::fetchAssoc() and ::fetchArray() are renamed to ::fetchAssociative() and ::fetchNumeric() respectively for consistency.
In order to make the Driver and the DBAL APIs distinct, a DBAL-level ResultStatement interface is introduced. It extends the one at the Driver level (therefore, it can gain new DBAL-level features w/o having to implement them at the Driver level).
The statements are no longer traversable. Only DBAL-level statements may be iterated via iterateNumeric(), iterateAssociative() or iterateColumn().
The StatementIterator class is removed.
Fetching in the “mixed”/“both” mode is no longer supported. It's not a limitation of the new design. I just haven't seen a single case in my entire life where it would be needed. Well… the caching layer could use it but it doesn't.

TODO:

Add the new methods to the wrapper connection class for consistency.
Provide the upgrade path and forward compatibility layer.
Adjust code comments and documentation.
Document the change in UPGRADE.md.

The next steps will be:

Separate driver-level API from the wrapper-level (Separate DBAL and driver interfaces #3591). This will allow independent evolution of the wrapper-level API w/o the need to make any changes to the drivers (e.g. add new fetch modes).
Replace ResultStatement with the Result interface. This will allow avoiding awkward situations like:
1. A statement exists but cannot be fetched from because it wasn't executed.
2. A component is given a statement to fetch the data from but it can bind new parameters to it and execute it.

greg0ire · 2020-05-14T06:14:28Z

Why not provide the upgrade path upfront and target 2.11.x? Since we do merges up now, it makes more sense to use that flow to keep branches in sync IMO, with a first PR that includes a BC layer and deprecations, a merge up PR, and a PR that drops deprecated code.

greg0ire

This new API is great, many bugs will be caught earlier thanks to it I think 👍

src/Driver/ResultStatement.php

src/Driver/FetchUtils.php

src/Driver/OCI8/OCI8Statement.php

morozov · 2020-05-15T04:39:31Z

Why not provide the upgrade path upfront and target 2.11.x?

Makes sense. I'd like to finish all the work here first, and before merging it, extract the relevant parts into a patch against 2.11.x.

morozov · 2020-05-21T06:14:50Z

Not sure how to proceed with the forward compatibility layer for 2.x. We can easily introduce the new methods in the wrapper connection class but what about the driver layer? Unlike a class where we can add methods, adding methods to the driver connection interface is a BC break on its own. If we just add them to the driver connection classes, then in order to use them, the code that depends on the interface will have to abuse its knowledge of the specific classes that implement the old interface and the new methods. Any ideas, @greg0ire?

greg0ire · 2020-05-21T08:03:09Z

You could create a new interface, and have the code that depends on the interface check if the new one is implemented, and complain if it isn't. Ideally, type declarations against the old interface should be dropped so that object can implement only the new interface. This should be ok because we require 7.2, so dropping a type declaration doesn't result in incompatible type declarations.

class ConsumingCode
{
    /** @param OldInterface|NewInterface $resultStatement */
    public function foo($resultStatement): void
    {
        if (!$resultStatement instanceof NewInterface) {
            // trigger deprecation
            // throw if $resultStatement does not implement Old Interface
            // use same code as before
            return;
        }


        // code with shiny new calls
        return;
    }
}

This plan leaves us with one issue: naming.

dbal/src/Driver/ResultStatement.php

Line 10 in cc7bc1e

* Interface for the reading part of a prepare statement only.

I can't say I understand this comment but maybe it will be of some inspiration to you for finding a new name?

morozov · 2020-05-21T15:23:04Z

This plan leaves us with one issue: naming.

The problem is with the design, not just with naming. See my notes in the description regarding ResultStatement. I already started prototyping the new API but the rework will affect a lot of code being changed here, so I decided to not proceed before this one is done.

morozov · 2020-05-21T15:29:58Z

Thanks for the code example, @greg0ire. It helps to understand the idea significantly. Let's see if it's feasible to apply it at the scale of all components that depend on the Statement interface.

greg0ire · 2020-05-27T07:08:23Z

The other PR I just merged seems to introduce conflicts

morozov · 2020-05-27T16:38:55Z

src/Cache/ResultCacheStatement.php


-        return $this->store($data);
+        return array_map('array_values', $this->data);


This will need to be backported to 2.11.x (found by PHPStan and enforced by the refactored ResultCacheTest).

morozov · 2020-05-27T17:33:47Z

@greg0ire it's ready now.

greg0ire · 2020-05-27T17:50:57Z

docs/en/reference/caching.rst

@@ -42,7 +42,7 @@ object is closed:

    <?php
    $stmt = $conn->executeCacheQuery($query, $params, $types, new QueryCacheProfile(0, "some key"));
-    $data = $stmt->fetchAll();
+    $data = $stmt->fetchAllAssociative();


Shouldn't the new docs be backported to the lower branch?

greg0ire · 2020-05-27T17:55:38Z

src/ForwardCompatibility/Driver/ResultStatement.php

- * Forward compatibility extension for the ResultStatement interface.
- */
-interface ResultStatement extends BaseResultStatement
-{


Should we maybe mark this as internal in lower branches? Since we remove it here, we don't want people to type hint against it. If it's ok to type hint against it, maybe we should keep it, deprecate it, and empty it.

Let's not mark it internal so that people don't feel wrong about using it, and let's empty and deprecate it instead of removal in 3.0.x.

Most likely, this interface will go away or get renamed/repurposed before 2.11.x is released but I'm fine with keeping it always releasable.

greg0ire

Not sure if it's the right day to use that emoji, but… 🚀

lcobucci · 2020-05-27T20:58:40Z

@morozov nice job, mate 👍

The API is much nicer and explicit but it has a huge impact on the ORM, so I think it's crucial to align these changes with @guilhermeblanco.

morozov · 2020-05-27T21:04:22Z

@lcobucci thanks for the heads up. I hope the impact is huge in terms of the number of LOC that need to be changed, not in terms of complexity. As far as I can tell, most of the code can be fixed almost by find and replace.

Since the same API is already implemented in 2.11.x, I believe we can proceed with the merge, and if there are any specific issues with the new API that need to be addressed, we'll have to address them starting 2.11.x anyways.

In the meanwhile, merging this will unblock me in reworking the driver/wrapper and statement/result interfaces.

Additional changes based on the discussion in #4007

… 2.11+ (xabbuh) This PR was merged into the 5.1 branch. Discussion ---------- [Messenger] fix forward compatibility with Doctrine DBAL 2.11+ | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | The methods will be deprecated in 2.11 (see doctrine/dbal#4019), but the forward compatibility layer is only present in 3.0 (see doctrine/dbal#4007). Commits ------- bca4f99 fix forward compatibility with Doctrine DBAL 2.11+

… 2.11+ (xabbuh) This PR was merged into the 5.1 branch. Discussion ---------- [Messenger] fix forward compatibility with Doctrine DBAL 2.11+ | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | The methods will be deprecated in 2.11 (see doctrine/dbal#4019), but the forward compatibility layer is only present in 3.0 (see doctrine/dbal#4007). Commits ------- bca4f9970b fix forward compatibility with Doctrine DBAL 2.11+

@gjdanis

Release [2.11.0](https://github.com/doctrine/dbal/milestone/77) 2.11.0 ====== - Total issues resolved: **7** - Total pull requests resolved: **55** - Total contributors: **8** Improvement,Prepared Statements,SQL Server,Types,pdo_sqlsrv,sqlsrv ------------------------------------------------------------------ - [4274: Support ASCII parameter binding](doctrine#4274) thanks to @gjdanis Documentation ------------- - [4271: Add explanation about implicit indexes](doctrine#4271) thanks to @greg0ire Deprecation,Error Handling -------------------------- - [4253: Deprecate DBAL\DBALException in favor of DBAL\Exception](doctrine#4253) thanks to @morozov CI -- - [4251: Setup automatic release workflow](doctrine#4251) thanks to @greg0ire Deprecation,Schema Managers --------------------------- - [4230: Deprecate the functionality of dropping client connections when dropping a database](doctrine#4230) thanks to @morozov Deprecation,Platforms --------------------- - [4229: Deprecate more AbstractPlatform methods](doctrine#4229) thanks to @morozov - [4132: Deprecate AbstractPlatform::fixSchemaElementName()](doctrine#4132) thanks to @morozov Improvement,Test Suite ---------------------- - [4215: Remove test group configuration leftovers](doctrine#4215) thanks to @morozov - [4080: Update PHPUnit to 9.2](doctrine#4080) thanks to @morozov - [4079: Forward compatibility with PHPUnit 9.3](doctrine#4079) thanks to @morozov - [3923: Removed performance tests](doctrine#3923) thanks to @morozov Deprecation,Schema ------------------ - [4213: Deprecate the Synchronizer package](doctrine#4213) thanks to @morozov Blocker,Improvement,PHP,Test Suite ---------------------------------- - [4201: Update PHPUnit to 9.3](doctrine#4201) thanks to @morozov Blocker,PHP,Test Suite ---------------------- - [4196: The test suite uses the deprecated at() matcher](doctrine#4196) thanks to @morozov Connections,Deprecation,Documentation ------------------------------------- - [4175: Additional deprecation note for PrimaryReplicaConnection::query()](doctrine#4175) thanks to @morozov Connections,Deprecation,Prepared Statements ------------------------------------------- - [4165: Deprecated usage of wrapper components as implementations of driver-level interfaces](doctrine#4165) thanks to @morozov - [4020: Deprecated Connection::project(), Statement::errorCode() and errorInfo()](doctrine#4020) thanks to @morozov Connections,Deprecation ----------------------- - [4163: Deprecate duplicate and ambiguous wrapper connection methods](doctrine#4163) thanks to @morozov Error Handling,Improvement,Types -------------------------------- - [4145: Add TypeRegistry constructor](doctrine#4145) thanks to @morozov Deprecation,Drivers,Improvement,pdo_mysql,pdo_oci,pdo_pgsql,pdo_sqlite,pdo_sqlsrv --------------------------------------------------------------------------------- - [4144: Deprecate classes in Driver\PDO* namespaces](doctrine#4144) thanks to @morozov Connections,Documentation,Improvement ------------------------------------- - [4139: Mark connection constructors internal](doctrine#4139) thanks to @morozov Deprecation,Drivers,Error Handling ---------------------------------- - [4137: Deprecate driver exception conversion APIs](doctrine#4137) thanks to @morozov - [4112: Deprecate DriverException::getErrorCode()](doctrine#4112) thanks to @morozov Configuration,Connections,Deprecation,Error Handling ---------------------------------------------------- - [4134: Deprecate some DBALException factory methods](doctrine#4134) thanks to @morozov Code Style,Documentation ------------------------ - [4133: Fix more issues introduced by the deprecation of driver classes](doctrine#4133) thanks to @morozov BC Break,Drivers,Error Handling,pdo_sqlsrv,sqlsrv ------------------------------------------------- - [4131: Restore the PortWithoutHost exception parent class](doctrine#4131) thanks to @morozov Code Style,Improvement,Static Analysis -------------------------------------- - [4123: Remove the no longer needed error suppressions](doctrine#4123) thanks to @morozov Deprecation,Drivers,Error Handling,Improvement ---------------------------------------------- - [4118: Deprecate ExceptionConverterDriver](doctrine#4118) thanks to @morozov Bug,Connections,Improvement,Prepared Statements ----------------------------------------------- - [4117: Fixes for the recently introduced driver-level deprecations](doctrine#4117) thanks to @morozov Connections,Deprecation,Platform Detection ------------------------------------------ - [4114: Deprecate ServerInfoAwareConnection#requiresQueryForServerVersion() as an implementation detail](doctrine#4114) thanks to @morozov Deprecation,Prepared Statements,SQL Parser,oci8 ----------------------------------------------- - [4110: Mark non-interface OCI8 driver methods internal](doctrine#4110) thanks to @morozov Connections,Deprecation,Drivers,Improvement,Prepared Statements --------------------------------------------------------------- - [4100: Deprecate inconsistently and ambiguously named driver-level classes](doctrine#4100) thanks to @morozov Connections,Improvement ----------------------- - [4092: Remove Connection::$isConnected](doctrine#4092) thanks to @morozov Configuration,Connections ------------------------- - [4086: Mark Connection::getParams() internal](doctrine#4086) thanks to @morozov Bug,Drivers,ibm_db2 ------------------- - [4085: The IBM DB2 driver Exception class must implement the DriverException interface](doctrine#4085) thanks to @morozov PHP --- - [4078: Bump PHP requirement to 7.3 as of DBAL 2.11.0](doctrine#4078) thanks to @morozov Connections,Databases,Deprecation,Drivers ----------------------------------------- - [4068: Deprecate Driver::getDatabase()](doctrine#4068) thanks to @morozov Deprecation,Improvement,Portability ----------------------------------- - [4061: Deprecated platform-specific portability mode constants](doctrine#4061) thanks to @morozov - [4054: &doctrine#91;doctrineGH-4052&doctrine#93; Deprecate MasterSlaveConnection and rename to PrimaryReplicaConnection](doctrine#4054) thanks to @beberlei and @albe - [4017: Improve help of dbal:run-sql command](doctrine#4017) thanks to @ostrolucky Code Style,Improvement ---------------------- - [4050: Update doctrine/coding-standard to 8.0](doctrine#4050) thanks to @morozov Cache,Deprecation,Improvement,Prepared Statements ------------------------------------------------- - [4049: Replace forward-compatible ResultStatement interfaces with Result](doctrine#4049) thanks to @morozov Cache,Improvement,Prepared Statements ------------------------------------- - [4048: Make caching layer not rely on closeCursor()](doctrine#4048) thanks to @morozov Deprecation,Improvement,Prepared Statements ------------------------------------------- - [4037: Introduce Statement::fetchFirstColumn()](doctrine#4037) thanks to @morozov - [4019: Deprecated the concept of the fetch mode](doctrine#4019) thanks to @morozov Bug,Documentation,Improvement,Prepared Statements ------------------------------------------------- - [4034: Additional changes based on the discussion in doctrine#4007](doctrine#4034) thanks to @morozov Connections,Console,Improvement ------------------------------- - [3956: allow using multiple connections for CLI commands](doctrine#3956) thanks to @dmaicher Deprecation,Logging ------------------- - [3935: Deprecate EchoSQLLogger](doctrine#3935) thanks to @morozov Improvement,Packaging --------------------- - [3924: Actualize the content of the .gitattributes file](doctrine#3924) thanks to @morozov Azure,Deprecation,Drivers,Drizzle,MariaDB,Platforms,PostgreSQL,SQL Anywhere,SQL Server,Sharding,pdo_ibm ------------------------------------------------------------------------------------------------------- - [3905: Deprecate the usage of the legacy platforms and drivers](doctrine#3905) thanks to @morozov Deprecation,Query ----------------- - [3864: CompositeExpression and()/or() factory methods](doctrine#3864) thanks to @BenMorel - [3853: Deprecate calling QueryBuilder methods with an array argument](doctrine#3853) thanks to @BenMorel and @morozov Deprecation,Tools ----------------- - [3861: Deprecated the usage of the Version class](doctrine#3861) thanks to @morozov Improvement,Query ----------------- - [3852: First parameter of ExpressionBuilder::and/or() mandatory](doctrine#3852) thanks to @BenMorel Deprecation,Improvement,Query ----------------------------- - [3851: Rename andX() / orX() methods](doctrine#3851) thanks to @BenMorel - [3850: Prepare CompositeExpression for immutability](doctrine#3850) thanks to @BenMorel # gpg: Signature made Mon Sep 21 01:47:31 2020 # gpg: using DSA key 1BEDEE0A820BC30D858F9F0C2C3A645671828132 # gpg: Can't check signature: No public key # Conflicts: # README.md # lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php # lib/Doctrine/DBAL/Driver/OCI8/Driver.php # lib/Doctrine/DBAL/Version.php

… 2.11+ (xabbuh) This PR was merged into the 5.1 branch. Discussion ---------- [Messenger] fix forward compatibility with Doctrine DBAL 2.11+ | Q | A | ------------- | --- | Branch? | 5.1 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | The methods will be deprecated in 2.11 (see doctrine/dbal#4019), but the forward compatibility layer is only present in 3.0 (see doctrine/dbal#4007). Commits ------- bca4f9970b fix forward compatibility with Doctrine DBAL 2.11+

morozov added WIP BC Break Prepared Statements labels May 13, 2020

morozov force-pushed the remove-fetch-modes branch 3 times, most recently from 9d630f8 to ef22d72 Compare May 13, 2020 17:23

morozov requested review from lcobucci, deeky666, Ocramius and greg0ire May 13, 2020 17:38

greg0ire reviewed May 14, 2020

View reviewed changes

src/Driver/ResultStatement.php Show resolved Hide resolved

src/Driver/FetchUtils.php Outdated Show resolved Hide resolved

src/Driver/OCI8/OCI8Statement.php Outdated Show resolved Hide resolved

morozov force-pushed the remove-fetch-modes branch from ef22d72 to 80c43f5 Compare May 15, 2020 04:34

morozov mentioned this pull request May 23, 2020

Deprecated the concept of the fetch mode #4019

Merged

4 tasks

morozov changed the base branch from master to 3.0.x May 27, 2020 01:18

morozov force-pushed the remove-fetch-modes branch 4 times, most recently from 43c5465 to b87249c Compare May 27, 2020 06:23

morozov removed the WIP label May 27, 2020

morozov requested a review from greg0ire May 27, 2020 06:32

morozov force-pushed the remove-fetch-modes branch 3 times, most recently from e5c49ba to 61831a5 Compare May 27, 2020 16:33

morozov commented May 27, 2020

View reviewed changes

morozov force-pushed the remove-fetch-modes branch from 9afc42d to 3a18d54 Compare May 27, 2020 17:21

greg0ire reviewed May 27, 2020

View reviewed changes

morozov added 2 commits May 27, 2020 12:18

Replace the concept of the statement fetch modes with an explicit API

96f8cab

Refactored ResultCacheTest for better coverage

9e3c56d

morozov force-pushed the remove-fetch-modes branch from 3a18d54 to 9e3c56d Compare May 27, 2020 19:19

morozov requested a review from greg0ire May 27, 2020 20:19

greg0ire approved these changes May 27, 2020

View reviewed changes

morozov requested a review from guilhermeblanco May 27, 2020 21:05

morozov merged commit 72e219a into doctrine:3.0.x May 27, 2020

morozov deleted the remove-fetch-modes branch May 27, 2020 21:59

morozov mentioned this pull request May 27, 2020

Additional changes based on the discussion in #4007 #4034

Merged

morozov added a commit that referenced this pull request May 28, 2020

Merge pull request #4034 from morozov/deprecate-fetch-mode

c08d3bc

Additional changes based on the discussion in #4007

morozov added this to the 3.0.0 milestone Jun 6, 2020

morozov self-assigned this Jun 6, 2020

xabbuh mentioned this pull request Jun 8, 2020

[Messenger] fix forward compatibility with Doctrine DBAL 2.11+ symfony/symfony#37141

Merged

greg0ire mentioned this pull request Apr 2, 2021

Please save fetch(), fetchAll(), and fetchColumn #4575

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace the concept of statement fetch modes with the explicit API #4007

Replace the concept of statement fetch modes with the explicit API #4007

morozov commented May 13, 2020 •

edited

Loading

greg0ire commented May 14, 2020

greg0ire left a comment

morozov commented May 15, 2020

morozov commented May 21, 2020

greg0ire commented May 21, 2020 •

edited

Loading

morozov commented May 21, 2020 •

edited

Loading

morozov commented May 21, 2020

greg0ire commented May 27, 2020

morozov May 27, 2020

morozov commented May 27, 2020

greg0ire May 27, 2020

morozov May 27, 2020

greg0ire May 27, 2020

morozov May 27, 2020

greg0ire left a comment

lcobucci commented May 27, 2020

morozov commented May 27, 2020


		return $this->store($data);
		return array_map('array_values', $this->data);

Replace the concept of statement fetch modes with the explicit API #4007

Replace the concept of statement fetch modes with the explicit API #4007

Conversation

morozov commented May 13, 2020 • edited Loading

greg0ire commented May 14, 2020

greg0ire left a comment

Choose a reason for hiding this comment

morozov commented May 15, 2020

morozov commented May 21, 2020

greg0ire commented May 21, 2020 • edited Loading

morozov commented May 21, 2020 • edited Loading

morozov commented May 21, 2020

greg0ire commented May 27, 2020

morozov May 27, 2020

Choose a reason for hiding this comment

morozov commented May 27, 2020

greg0ire May 27, 2020

Choose a reason for hiding this comment

morozov May 27, 2020

Choose a reason for hiding this comment

greg0ire May 27, 2020

Choose a reason for hiding this comment

morozov May 27, 2020

Choose a reason for hiding this comment

greg0ire left a comment

Choose a reason for hiding this comment

lcobucci commented May 27, 2020

morozov commented May 27, 2020

morozov commented May 13, 2020 •

edited

Loading

greg0ire commented May 21, 2020 •

edited

Loading

morozov commented May 21, 2020 •

edited

Loading